Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[easy] fixed missing include #3231

Merged
merged 1 commit into from
Aug 3, 2022
Merged

[easy] fixed missing include #3231

merged 1 commit into from
Aug 3, 2022

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Aug 3, 2022

Surprisingly, the issue was not detected before
probably because "fileio.h" is always included after "util.h".

I presume it was not detected before
because "fileio.h" is probably always included after "util.h".
@Cyan4973 Cyan4973 changed the title minor : fixed missing include [easy] fixed missing include Aug 3, 2022
Cyan4973 added a commit that referenced this pull request Aug 3, 2022
fileio_types.h cannot be parsed by itself
because it relies on basic types defined in `lib/common/mem.h`.
As for #3231, it likely wasn't detected because `mem.h` was probably included before within target files.
But this is not proper.

A "easy" solution would be to add the missing include,
but each dependency should be considered "bad" by default,
and only allowed if it brings some tangible value.

In this case, since these types are only used to declare internal structure variables
which are effectively only flags,
I believe it's really not valuable to add a dependency on `mem.h` for this purpose
while the standard `int` type can do the same job.

I was expecting some compiler warnings following this change,
but it turns out we don't use `-Wconversion` by default on `zstd` source code,
so there is none.

Nevertheless, I enabled `-Wconversion` locally and proceeded to fix a few conversion warnings in the process.

Adding `-Wconversion` to the list of flags used for `zstd` is something I would be favorable over the long term,
but it cannot be done overnight,
because the nb of places where this warning is triggered is daunting.
Better progressively reduce the nb of triggered `-Wconversion` warnings before enabling this flag by default.
@Cyan4973 Cyan4973 merged commit 9e90b18 into dev Aug 3, 2022
@Cyan4973 Cyan4973 deleted the fileio_missingInclude branch January 13, 2023 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants